Re: [Xen-devel] [PATCH v5 1/1] Add mmio_hole_size

[George Dunlap <dunlapg@xxxxxxxxx>]

On Wed, Oct 1, 2014 at 10:11 AM, Slutz, Donald Christopher
<dslutz@xxxxxxxxxxx> wrote:
> On 09/30/14 09:22, George Dunlap wrote:
>> On Tue, Sep 30, 2014 at 2:27 AM, Boris Ostrovsky
>> <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>> +    if ( mmio_hole_size )
>>>> +    {
>>>> +        uint64_t max_ram_below_4g = (1ULL << 32) - mmio_hole_size;
>>>> +
>>>> +        if ( max_ram_below_4g > HVM_BELOW_4G_MMIO_START )
>>>> +        {
>>>> +            printf("max_ram_below_4g=0x"PRIllx
>>>> +                   " too big for mmio_hole_size=0x"PRIllx
>>>> +                   " has been ignored.\n",
>>>> +                   PRIllx_arg(max_ram_below_4g),
>>>> +                   PRIllx_arg(mmio_hole_size));
>>>> +        }
>>>
>>> Do you need to check whether the hole is too large?
>>>
>>> Here and in the toolstack.
>> How large is too large?  I've seen real machines with a 3GiB memory hole...
>>
>>>> "0";
>>>> +        if (info->u.hvm.mmio_hole_size) {
>>>> +            uint64_t max_ram_below_4g =
>>>> +                (1ULL << 32) - info->u.hvm.mmio_hole_size;
>>>> +
>>>> +            if (max_ram_below_4g <= HVM_BELOW_4G_MMIO_START) {
>>>
>>> You seem to be making various checks on hole size in multiple places ---
>>> here, in libxl__build_device_model_args_new() and in parse_config_data().
>>> (And, to some degree, in xc_hvm_build_with_hole()). Are they all necessary?
>> I think we should get rid of xc_hvm_build_with_hole() entirely, but I
>> think we want a check everywhere else.  The key question is when the
>> failure is going to happen ideally.  You absolutely have to check it
>> at the bottom level (hvmloader).  But you don't want to go through all
>> the hassle of setting up the domain and starting to boot it only to
>> find that some silly parameter is messed up, so it should also be
>> checked in libxl (since many people will be going through only libxl).
>> But if you're using xl and get an error at the libxl layer, the error
>> message is often a bit cryptic.  So it makes sense to have it at the
>> parsing level too.
>>
>>   -George
>
> Ok, I will look into removing xc_hvm_build_with_hole() entirely.
>
> The rest of the statement looks like stuff I should have had in the
> commit message.  Are you fine with me adjusting and adding it?

Er, I'm not sure what this question is about. :-)  I don't think you
need information about where things are checked in the commit message
-- you mainly just need to give people information about what problem
you're solving and a high-level idea what it's doing; and sometimes an
idea which code the patch is touching if it's not clear.  I think your
commit message is probably fine as it is.

 -George

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