Re: [Xen-devel] [PATCH v2 03/17] x86/hvm: unify internal portio and mmio intercepts

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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