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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.04.16 at 14:00, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:

> 
> On 4/28/2016 7:52 PM, Jan Beulich wrote:
>>>>> On 28.04.16 at 13:40, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>>>>>> On 28.04.16 at 12:42, <george.dunlap@xxxxxxxxxx> wrote:
>>>>> 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. :)
>>
>> gcc will like it, but as said clang won't (afair).
>>
>>>>             [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.
>>
>> No. As said, I want to avoid explicit initializers for unused slots,
>> and hence it has to be zero that gets checked against.
>>
> 
> But "!memtype[a.hvmmem_type]" is indicating the p2m type is p2m_ram_rw,
> which should be allowed here...

Hence the additional check for a.hvmmem_type to not be zero
(that's the only thing mapping to p2m_ram_rw).

Jan


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