Re: [Xen-devel] [PATCH v2 23/25] arm/altp2m: Extend libxl to activate altp2m on ARM.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Wei,

On 08/11/2016 06:00 PM, Wei Liu wrote:
> Sorry for the late reply.
> 

No worries, it's all good :) Thanks for your reply.

[...]

>>>>                                         ("smbios_firmware",  string),
>>>>                                         ("acpi_firmware",    string),
>>>>                                         ("hdtype",           libxl_hdtype),
>>>> @@ -561,6 +560,9 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>>  
>>>>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> 
> But you do need to wrap the line properly.

I am not sure whether this comment was intended, as the upper line
containing the fields arch_arm and gic_version were not changed by the
patch.

>>>>                                ])),
>>>> +    # Alternate p2m is not bound to any architecture or guest type, as it 
>>>> is
>>>> +    # supported by x86 HVM and ARM PV guests.
>>> Just "ARM guests" would do. ARM doesn't have notion of PV vs HVM.
>>
>> I will change that, thank you. I mentioned ARM PV as currently it is the
>> type that is registered for guests on ARM.
>>
>>>> +    ("altp2m", libxl_defbool),
>>>>  
>>>>      ], dir=DIR_IN
>>>>  )

[...]

>>>>  
>>>>          xlu_cfg_replace_string(config, "smbios_firmware",
>>>>                                 &b_info->u.hvm.smbios_firmware, 0);
>>>> @@ -1727,6 +1732,25 @@ static void parse_config_data(const char 
>>>> *config_source,
>>>>          abort();
>>>>      }
>>>>  
>>>> +    bool altp2m_support = false;
>>>> +#if defined(__i386__) || defined(__x86_64__)
>>>> +    /* Alternate p2m support on x86 is available only for HVM guests. */
>>>> +    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM)
>>>> +        altp2m_support = true;
>>>> +#elif defined(__arm__) || defined(__aarch64__)
>>>> +    /* Alternate p2m support on ARM is available for all guests. */
>>>> +    altp2m_support = true;
>>>> +#endif
>>>> +
>>> I don't think you need to care too much about dead option here.
>>> Xl should be able to set altp2m field all the time. 
>>
>> I am actually not sure what you mean by the dead option. Could you be
>> more specific, please?
>>  
>> Also, in the last patch, we came to the agreement to guard the altp2m
>> functionality solely through the HVM param (instead of the additional
>> Xen cmd-line option altp2m), which is set through libxl. Because of
>> this, the entire domu initialization routines depend on this option to
>> be set at the moment of domain creation -- not after it. That is, being
>> able to set the altp2m option at all time would actually break the system.
>>
> 
> Why is this? It's possible we're talking past each other.
> 

I believe I was not entirely clear in my upper comment. By "we", I meant
all parties involved into the discussion concerning altp2m on ARM in
general. Sorry, for the confusion.

Also, I was wrong: The HVM param can indeed be set at any time without
any troubles.

>>> And there should be
>>> code in libxl to handle situation when altp2m is not available.
>>
>> I am not so sure about that either:
>>
>> Currently, altp2m is supported on x86 HVM and on ARM.
>>  * on x86, altp2m depends on HW to support altp2m. Therefore, the
>> cmd-line option "altp2m" is used do activate altp2m. All libxl
>> interaction with the altp2m subsystem will be discarded at this point.
>>  * on ARM, altp2m is implemented purely in SW. That is, we do not have
>> ARM architectures, that would not support altp2m -- so the idea.
>>  * All other architectures should not be able to activate altp2m, which
>> is why I chose the #defines (__arm__, __x86_64__, ...) to guard the
>> upper option.
>>
>>>> +    if (altp2m_support) {
>>>> +        /* The config parameter "altp2m" replaces the parameter 
>>>> "altp2mhvm".
>>>> +         * For legacy reasons, both parameters are accepted on x86 HVM 
>>>> guests
>>>> +         * (only "altp2m" is accepted on ARM guests). If both parameters 
>>>> are
>>>> +         * given, it must be considered that the config parameter 
>>>> "altp2m" will
>>>> +         * always have priority over "altp2mhvm". */
>>>> +        xlu_cfg_get_defbool(config, "altp2m", &b_info->altp2m, 0);
>>>> +    }
>>>> +
> 
> 
> What I meant was:
> 
> We don't care xl (the application) sets altp2m or not. It's entirely possible
> that the xl on its own sets that field. The validation should be done
> inside libxl. If a particular configuration is not supported by libxl,
> it should reject that. Libxl shouldn't rely on xl (the application) to
> pass in fully sanitised data, it should sanitise the input itself.
> 
> Basically that means, in xl, you only need:
> 
>     +     xlu_cfg_get_defbool(config, "altp2m", &b_info->altp2m, 0);
> 
> And then in libxl:initiate_domain_create you check if the configuration
> is valid. You can see a bunch of other checks are already there.
> 
> Does this make sense to you?
> 
> 

I think so. As you said, in my next patch, I will simply set
b_info->u.hvm.altp2m and b_info->altp2m during parsing and check for
sufficient support in initiate_domain_create. Thank you.


>>> As always, if what I said above doesn't make sense to you, feel free to
>>> ask.
>>

Best regards,
~Sergej

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