Re: [Xen-devel] [PATCH v9 07/11] vpci/bars: add handlers to map the BARs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.03.18 at 15:04, <roger.pau@xxxxxxxxxx> wrote:
> +static void modify_decoding(const struct pci_dev *pdev, bool map, bool 
> rom_only)
> +{
> +    struct vpci_header *header = &pdev->vpci->header;
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    unsigned int i;
> +
> +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +    {
> +        if ( rom_only && header->bars[i].type == VPCI_BAR_ROM )
> +        {
> +            unsigned int rom_pos = (i == PCI_HEADER_NORMAL_NR_BARS)
> +                                   ? PCI_ROM_ADDRESS : PCI_ROM_ADDRESS1;
> +            uint32_t val = pci_conf_read32(pdev->seg, pdev->bus, slot, func,
> +                                           rom_pos);
> +
> +            header->bars[i].enabled = header->rom_enabled = map;
> +
> +            val &= ~PCI_ROM_ADDRESS_ENABLE;
> +            val |= map ? PCI_ROM_ADDRESS_ENABLE : 0;
> +            pci_conf_write32(pdev->seg, pdev->bus, slot, func, rom_pos, val);
> +            break;
> +        }
> +        if ( !rom_only && (header->bars[i].type != VPCI_BAR_ROM ||
> +                           header->rom_enabled) )
> +            header->bars[i].enabled = map;

While this second if() has benefited from the rename to "rom_only",
I'm now wondering about the validity of the first if(): Why would
this need doing only in the "ROM only" case, but not in the
"everything" one? Or is the parameter still suffering from its name
being misleading? This also needs to be viewed in context of the
call here from vpci_process_pending(), which passes (dropping
the conditional there) v->vpci.rom, which doesn't exactly mean
"ROM only".

If there's really no name for the parameter that can properly
convey its meaning, please attach a clarifying comment. (Having
reached the end of the patch I now seem to understand / recall
that this is for the case where the ROM BAR's enable bit changes.
That's what a comment could usefully say here.)

> +    }
> +
> +    if ( !rom_only )
> +    {

Note how due to this conditional the "break" above could
actually be "return", making more obvious that the rest of the
function isn't be needed in that case.

> +static int maybe_defer_map(struct domain *d, struct pci_dev *pdev,
> +                           struct rangeset *mem, bool map, bool rom)
> +{
> +    struct vcpu *curr = current;
> +    int rc;
> +
> +    if ( is_idle_vcpu(curr) )
> +    {
> +        struct map_data data = { .d = d, .map = true };
> +
> +        /*
> +         * Dom0 building runs on the idle vCPU, in which case it's not 
> possible
> +         * to defer the operation (like done in the else branch). Call
> +         * rangeset_consume_ranges in order to establish the mappings right
> +         * away.
> +         */

For one I think this comment belongs above the if(), as that's what
it explains, not the ASSERT() that follows. And then it clarifies only
half of what needs clarifying: Why can't we make it here on an idle
vCPU outside of Dom0 building (e.g. through a tasklet), or if we can,
why is the given behavior the intended one?

> +        ASSERT(map && !rom);

I can see why you assume it's not an un-mapping request (albeit
I wonder whether you couldn't instead simply set .map above to
the incoming value), but why the !rom part?

> +static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
> +{
> +    struct vpci_header *header = &pdev->vpci->header;
> +    struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> +    struct pci_dev *tmp, *dev = NULL;
> +    unsigned int i;
> +    int rc;
> +
> +    if ( !mem )
> +        return -ENOMEM;
> +
> +    /*
> +     * Create a rangeset that represents the current device BARs memory 
> region
> +     * and compare it against all the currently active BAR memory regions. If
> +     * an overlap is found, subtract it from the region to be
> +     * mapped/unmapped.
> +     *
> +     * NB: the rangeset uses inclusive frame numbers.

Is this a worthwhile remark to make? All rangesets do, so if at all
that's what the comment should say.

> +     */
> +
> +    /*
> +     * First fill the rangeset with all the BARs of this device or with the 
> ROM
> +     * BAR only, depending on whether the guest is toggling the memory decode
> +     * bit of the command register, or the enable bit of the ROM BAR 
> register.
> +     */
> +    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> +    {
> +        const struct vpci_bar *bar = &header->bars[i];
> +
> +        if ( !MAPPABLE_BAR(bar) ||
> +             (rom_only ? bar->type != VPCI_BAR_ROM
> +                       : (bar->type == VPCI_BAR_ROM && 
> !header->rom_enabled)) )
> +            continue;
> +
> +        rc = rangeset_add_range(mem, PFN_DOWN(bar->addr),
> +                                PFN_UP(bar->addr + bar->size - 1));
> +        if ( rc )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "Failed to add [%" PRI_gfn ", %" PRI_gfn "]: %d\n",
> +                   PFN_DOWN(bar->addr), PFN_UP(bar->addr + bar->size - 1),

I thought we had agreed that the parenthesization of tuples
like this one should match meaning they want to convey. I'm
having a hard time to see how PFN_UP() could ever go together
with a closing square bracket.

> +static void cmd_write(const struct pci_dev *pdev, unsigned int reg,
> +                      uint32_t cmd, void *data)
> +{
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    uint16_t current_cmd = pci_conf_read16(pdev->seg, pdev->bus, slot, func,
> +                                           reg);
> +
> +    /*
> +     * Let Dom0 play with all the bits directly except for the memory
> +     * decoding one.
> +     */
> +    if ( (cmd ^ current_cmd) & PCI_COMMAND_MEMORY )
> +        /*
> +         * Ignore the error. No memory has been added or removed from the 
> p2m,
> +         * and the memory decoding has not been changed, so leave everything
> +         * as-is, hoping the guest will realize and try again.
> +         */
> +        modify_bars(pdev, cmd & PCI_COMMAND_MEMORY, false);

So that comment appears to be correct, but I wonder if the reader
could get a little more assistance, as it's not exactly obvious why no
p2m changes would have occurred in case of failure: modify_bars()
produces all its errors before doing any mapping, and
maybe_defer_map() takes the "else" branch which doesn't cause
any (direct) errors. Same for the similar comment in rom_write().

> +#else /* !CONFIG_HAS_VPCI */
> +struct vpci_vcpu {
> +};
> +#endif

To make clear even from e.g. simple grep output that this is a
dummy, could I talk you into making this

#else /* !CONFIG_HAS_VPCI */
struct vpci_vcpu {};
#endif

?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel