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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.18 at 14:59, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Mar 15, 2018 at 06:41:00AM -0600, Jan Beulich wrote:
>> >>> On 15.03.18 at 12:33, <roger.pau@xxxxxxxxxx> wrote:
>> > On Wed, Mar 14, 2018 at 10:13:16AM -0600, Jan Beulich wrote:
>> >> >>> On 14.03.18 at 15:04, <roger.pau@xxxxxxxxxx> wrote:
>> >> > +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?
>> > 
>> > Since this seems to be causing confusion, what about using:
>> > 
>> > system_state != SYS_STATE_active
>> > 
>> > Instead of checking if running on the idle vpcu. Do you think that
>> > would make it clearer?
>> 
>> Yes, I think so. That would then make clear that if you moved the
>> conditional into the only caller and split the function, the Dom0 one
>> could even become __init.
> 
> I've splitted the function into defer_map and apply_map, and added the
> following at the end of modify_bars:
> 
> if ( system_state != SYS_STATE_active )

Careful here, btw: You really mean "system_state < SYS_STATE_active",
as domains get frozen only _after_ setting system_state to
SYS_STATE_suspend (but perhaps it's worth considering whether
to change that).

> {
>     /*
>      * Mappings might be created when building Dom0 if the memory decoding
>      * bit of PCI devices is enabled. In that case it's not possible to
>      * defer the operation, so call apply_map in order to create the
>      * mappings right away. Note that at build time this function will only
>      * be called iff the memory decoding bit is enabled, thus the operation
>      * will always be to establish mappings and process all the BARs.
>      */
>     ASSERT(map && !rom_only);
>     return apply_map(pdev->domain, dev, mem);
> }
> 
> defer_map(pdev->domain, dev, mem, map, rom_only);
> 
> 
> Would you also like me to change the is_idle_vcpu check in map_range
> to use system_state == SYS_STATE_active also?

Well, if it controls the same thing, then both should match up.

Jan


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