[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 03/17] x86/hvm: unify internal portio and mmio intercepts
>>> On 17.06.15 at 16:40, <Paul.Durrant@xxxxxxxxxx> wrote: >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] >> Sent: 17 June 2015 15:23 >> >>> On 11.06.15 at 17:42, <paul.durrant@xxxxxxxxxx> wrote: >> > + case IOREQ_TYPE_COPY: >> > + if ( p->df ) >> > { >> > - if ( vio->mmio_large_read_bytes != p->size ) >> > - return X86EMUL_UNHANDLEABLE; >> > - memcpy(&data, vio->mmio_large_read, p->size); >> > - vio->mmio_large_read_bytes = 0; >> > - vio->mmio_retrying = 0; >> > + start = (p->addr - (p->count - 1) * p->size); >> >> You're re-introducing a problem here that I fixed not so long ago: >> The way it's coded here the multiplication is a 32-bit unsigned one, >> but we need it to be a 64-bit signed one. I.e. you need 1L here >> (and elsewhere) instead of just 1. > > Ok. That's clearly a bit subtle though, since I missed it. Would it be more > obvious to have something like: > > start = p->addr - (uint64_t)p->size * (p->count - 1); Depends on the person looking at it: I generally advocate for avoiding casts whenever possible. Hence the 1L conversion I did a while back. (Apart from that your code would break again once it would be used in a hypothetical 128-bit environment, while my variant would - assuming long would then be 128 bits wide - still work.) >> And is _all_ of this really usefully per-domain anyway? >> > > Maybe the portio handlers could go global too but it seems more flexible to > make things per-domain. Maybe, in the long run. >> > +#define NR_MMIO_HANDLERS 5 >> > >> > -#define HVM_MMIO_HANDLER_NR 5 >> > +struct hvm_mmio_handler { >> > + const struct hvm_mmio_ops *ops; >> > + struct hvm_io_handler io_handler; >> > +}; >> > >> > -int hvm_io_intercept(ioreq_t *p, int type); >> > -void register_io_handler( >> > - struct domain *d, unsigned long addr, unsigned long size, >> > - void *action, int type); >> > -void relocate_io_handler( >> > - struct domain *d, unsigned long old_addr, unsigned long new_addr, >> > - unsigned long size, int type); >> > +typedef int (*portio_action_t)( >> > + int dir, uint32_t port, uint32_t bytes, uint32_t *val); >> > + >> > +#define NR_PORTIO_HANDLERS 16 >> > + >> > +struct hvm_portio_handler { >> > + uint64_t start, end; >> >> Why uint64_t? (Note that this is simply the place I spotted this first >> at - the problem appears to exist in all of the port I/O handling.) > > Because the common process loop and other functions deal in 64-bit vals so > that they don't have to care whether they are port or mmio address values. > They are cast down to 32-bits before the action is invoked. There's nothing "common" here - even if common code deals with uint64_t-s, these fields could be uint32_t, unsigned int, or even uint16_t. No need for them to take up more space than needed. Same for function parameters - unless their type is dictated by a structure field they want to be stored into, port-I/O specific functions should take unsigned int port numbers to produce better code. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |