Prev: fujitsu-laptop: remove unnecessary input_free_device
Next: [PATCH] time/fs - file's time race with vgettimeofday
From: Jin Dongming on 2 Jul 2010 03:40
Hi, Xiao Guangrong

Xiao Guangrong wrote:
> The IOAPIC spec says:
>
> When accessing these registers, accesses must be done one dword at a time.
> For example, software should never access byte 2 from the Data register before
> accessing bytes 0 and 1. The hardware will not attempt to recover from a bad
> programming model in this case.

I could understand what you described on the above, but I don't think it is
the best method to fix it. Could you consider a nice one?

>
> So, this patch removes other width access
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong(a)cn.fujitsu.com>
> ---
> virt/kvm/ioapic.c | 10 +++-------
> 1 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 1149c60..f96816f 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -306,14 +306,10 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
> spin_unlock(&ioapic->lock);
>
> switch (len) {
> - case 8:
> - *(u64 *) val = result;
> - break;
> - case 1:
> - case 2:
> case 4:
> - memcpy(val, (char *)&result, len);

Here the parameter len is not used for reading data from ioapic register, before switch case
the value of ioapic register has been read by ioapic_read_indirect().


> + *(u32 *) val = result;
> break;
> +
> default:
> printk(KERN_WARNING "ioapic: wrong length %d\n", len);

And I think here is not good to print warning message. Maybe it is better to do
such kind of checking at the first of this function before ioapic_read_indirect().

> }
> @@ -332,7 +328,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
> (void*)addr, len, val);
> ASSERT(!(addr & 0xf)); /* check alignment */
>
> - if (len == 4 || len == 8)
> + if (len == 4)
> data = *(u32 *) val;
> else {
> printk(KERN_WARNING "ioapic: Unsupported size %d\n", len);

So I hope you could read the source code again. I think you can use a better method to fix it.

Best Regards,
Jin Dongming

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Xiao Guangrong on 2 Jul 2010 03:50


Jin Dongming wrote:

>>
>> switch (len) {
>> - case 8:
>> - *(u64 *) val = result;
>> - break;
>> - case 1:
>> - case 2:
>> case 4:
>> - memcpy(val, (char *)&result, len);
>
> Here the parameter len is not used for reading data from ioapic register, before switch case
> the value of ioapic register has been read by ioapic_read_indirect().
>

Yeah, it's right, but it's rarely operation that guest using incorrect width to access
the registers, so i don't think it's worthy to move the width judgment before the real
registers read.

>
>> + *(u32 *) val = result;
>> break;
>> +
>> default:
>> printk(KERN_WARNING "ioapic: wrong length %d\n", len);
>
> And I think here is not good to print warning message. Maybe it is better to do
> such kind of checking at the first of this function before ioapic_read_indirect().
>

ditto

Thanks for your review, Jin!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Marcelo Tosatti on 2 Jul 2010 13:50
On Fri, Jul 02, 2010 at 04:00:12PM +0800, Xiao Guangrong wrote:
> The IOAPIC spec says:
>
> When accessing these registers, accesses must be done one dword at a time.
> For example, software should never access byte 2 from the Data register before
> accessing bytes 0 and 1. The hardware will not attempt to recover from a bad
> programming model in this case.
>
> So, this patch removes other width access
>
> Signed-off-by: Xiao Guangrong <xiaoguangrong(a)cn.fujitsu.com>
> ---
> virt/kvm/ioapic.c | 20 +++++++-------------
> 1 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 1149c60..6610d11 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -288,6 +288,11 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
> ioapic_debug("addr %lx\n", (unsigned long)addr);
> ASSERT(!(addr & 0xf)); /* check alignment */
>
> + if (len != 4) {
> + printk(KERN_WARNING "ioapic: wrong length %d\n", len);
> + return 0;
> + }
> +

Just remove the printks please, as guests can flood hosts dmesg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: fujitsu-laptop: remove unnecessary input_free_device
Next: [PATCH] time/fs - file's time race with vgettimeofday