Re: [Xen-devel] [PATCH v4 07/17] x86/hvm: add length to mmio check op

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.06.15 at 15:34, <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 24 June 2015 14:25
>> To: Paul Durrant
>> Cc: Andrew Cooper; xen-devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
>> Subject: RE: [Xen-devel] [PATCH v4 07/17] x86/hvm: add length to mmio
>> check op
>> 
>> >>> On 24.06.15 at 15:14, <Paul.Durrant@xxxxxxxxxx> wrote:
>> >> From: xen-devel-bounces@xxxxxxxxxxxxx [mailto:xen-devel-
>> >> bounces@xxxxxxxxxxxxx] On Behalf Of Jan Beulich
>> >> Sent: 24 June 2015 14:08
>> >> >>> On 24.06.15 at 13:24, <paul.durrant@xxxxxxxxxx> wrote:
>> >> > --- a/xen/include/asm-x86/hvm/io.h
>> >> > +++ b/xen/include/asm-x86/hvm/io.h
>> >> > @@ -35,7 +35,9 @@ typedef int (*hvm_mmio_write_t)(struct vcpu *v,
>> >> >                                  unsigned long addr,
>> >> >                                  unsigned long length,
>> >> >                                  unsigned long val);
>> >> > -typedef int (*hvm_mmio_check_t)(struct vcpu *v, unsigned long
>> addr);
>> >> > +typedef int (*hvm_mmio_check_t)(struct vcpu *v,
>> >> > +                                unsigned long addr,
>> >> > +                                unsigned long length);
>> >>
>> >> I don't think this really needs to be "long"?
>> >>
>> >
>> > For consistency with the mmio read and write function types I went with
>> > 'long'. Is there any harm in that?
>> 
>> Generally generates worse code (due to the need for the REX64
>> prefix on all involved instructions). Perhaps the other ones don't
>> need sizes/lengths passed as longs either?
>> 
> 
> I'm happy to do it that way round if you don't mind the extra diffs. I'll do 
> it as a separate patch just before this one, to ease review.

Thanks. There's actually a good example of why they shouldn't be
unsigned long in patch 9 I just looked at:

+static int stdvga_mem_write(struct vcpu *v, unsigned long addr,
+                            unsigned long size, unsigned long data)
 {
+    ioreq_t p = { .type = IOREQ_TYPE_COPY,
+                  .addr = addr,
+                  .size = size,

This clearly would truncate size if it ever exceeded a uint32_t (and
hence an unsigned int on x86).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel