Re: [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

["Yu, Zhang" <yu.c.zhang@linux.intel.com>]

Thanks Jan. And I admire your rigorous thought. :)

On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>> On 28.04.16 at 12:42, <george.dunlap@citrix.com> wrote:
>> On 28/04/16 11:22, Jan Beulich wrote:
>>>>>> On 28.04.16 at 10:29, <yu.c.zhang@linux.intel.com> wrote:
>>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>              [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>>              [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>>              [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>>> -            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>>> +            [HVMMEM_unused] = p2m_invalid
>>>
>>> Why don't you simply delete the old line, without replacement?
>>

Well, I did not delete the old line, because in my coming patch(the
p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
against HVMMEN_unused later in this routine appear in that patch.

>> That might have been slightly cleaner; but we're going to have to put it
>> back as soon as the development window opens anyway, so I don't really
>> see the point of going through the effort of respinning the patch again.
>>
>> Would you be willing to ack this version anyway?
>
> I have no problem doing so (and in fact I have it on my to by
> committed list already), it is just looked slightly confusing (and
> I had already typed half a reply that this isn't what was discussed
> until I properly looked at the next hunk), and hence I wanted to
> understand the motivation. And btw., I'm not convinced it would
> need to be put there anyway later: I don't view the used
> mechanism as a good (read: extensible) one to deal with what
> would be holes in the array above. Indeed we can't leave them
> uninitialized (as that would mean p2m_ram_rw), but I think we
> should better find a way to initialize _all_ unused slots without
> requiring an initializer for each of them. Sadly the desire to allow
> compilation with clang prohibits the most natural solution:
>
>         static const p2m_type_t memtype[] = {
>             [0 ... <upper-bound> - 1] = p2m_invalid,

Not sure if this will compile? Can have a try. :)

>             [HVMMEM_ram_rw]  = p2m_ram_rw,
>             [HVMMEM_ram_ro]  = p2m_ram_ro,
>             [HVMMEM_mmio_dm] = p2m_mmio_dm,
>         };
>
> Maybe we could do (altering the second hunk of this patch)
>
> @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>               ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>              goto setmemtype_fail;
>
> -        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
> +        BUILD_BUG_ON(p2m_ram_rw);
> +        BUILD_BUG_ON(HVMMEM_ram_rw);
> +        if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
> +             (a.hvmmem_type && !memtype[a.hvmmem_type]) )

I guess by !memtype[a.hvmmem_type] you are trying to check if it's
p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
should be checked like memtype[a.hvmmem_type] < 0 and initialize the
holes with -1.

But I still wonder is this really necessary? Because we only have one
hole in this array in the forseeable future.

>              goto setmemtype_fail;
>
>          while ( a.nr > start_iter )
>
> Jan
>
>

B.R.
Yu

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel