Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 01, 2022 at 07:31:31AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> On 31.01.22 17:50, Jan Beulich wrote:
> > On 31.01.2022 16:06, Oleksandr Andrushchenko wrote:
> >> Hi, Roger!
> >>>>                rom->type = VPCI_BAR_EMPTY;
> >>>>        }
> >>>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> >>>> index ed127a08a953..0a73b14a92dc 100644
> >>>> --- a/xen/include/xen/vpci.h
> >>>> +++ b/xen/include/xen/vpci.h
> >>>> @@ -68,7 +68,10 @@ struct vpci {
> >>>>        struct vpci_header {
> >>>>            /* Information about the PCI BARs of this device. */
> >>>>            struct vpci_bar {
> >>>> +            /* Physical view of the BAR. */
> >>> No, that's not the physical view, it's the physical (host) address.
> >>>
> >>>>                uint64_t addr;
> >>>> +            /* Guest view of the BAR: address and lower bits. */
> >>>> +            uint64_t guest_reg;
> >>> I continue to think it would be clearer if you store the guest address
> >>> here (gaddr, without the low bits) and add those in guest_bar_read
> >>> based on bar->{type,prefetchable}. Then it would be equivalent to the
> >>> existing 'addr' field.
> >>>
> >> I agreed first to do such a change, but then recalled our discussion with 
> >> Jan [1].
> >> And then we decided that in order for it to be efficient it is better if 
> >> we setup all the
> >> things during the write phase (rare), rather then during the write phase 
> >> (more often).
> > Small correction: The 2nd "write" was likely meant to be "read".
> Yes, this is correct.
> >   But
> > please recall that Roger is the maintainer of the code, so he gets
> > the final say.
> Agree, but would vote for the current approach as it still saves some
> CPU cycles making the read operation really tiny

I think you need to build the mapping rangeset(s) based on guest
addresses, not host ones, so it's likely going to be easier if you
store the address here in order to use it when building the rangeset.

Overall the cost of the vmexit will shadow the cost of doing a couple
of ORs here in order to return the guest view of the BAR.

If you think storing the guest view of the BAR register will make the
code easier to understand, then please go ahead. Otherwise I would
recommend to store the address like we do for the host position of the
BAR (ie: addr field).

Thanks, Roger.