[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for-4.12 2/8] amd/ntp: remove assert that prevents creating 2M MMIO entries


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>
  • From: George Dunlap <george.dunlap@xxxxxxxxxx>
  • Date: Thu, 7 Feb 2019 17:21:28 +0000
  • Autocrypt: addr=george.dunlap@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFPqG+MBEACwPYTQpHepyshcufo0dVmqxDo917iWPslB8lauFxVf4WZtGvQSsKStHJSj 92Qkxp4CH2DwudI8qpVbnWCXsZxodDWac9c3PordLwz5/XL41LevEoM3NWRm5TNgJ3ckPA+J K5OfSK04QtmwSHFP3G/SXDJpGs+oDJgASta2AOl9vPV+t3xG6xyfa2NMGn9wmEvvVMD44Z7R W3RhZPn/NEZ5gaJhIUMgTChGwwWDOX0YPY19vcy5fT4bTIxvoZsLOkLSGoZb/jHIzkAAznug Q7PPeZJ1kXpbW9EHHaUHiCD9C87dMyty0N3TmWfp0VvBCaw32yFtM9jUgB7UVneoZUMUKeHA fgIXhJ7I7JFmw3J0PjGLxCLHf2Q5JOD8jeEXpdxugqF7B/fWYYmyIgwKutiGZeoPhl9c/7RE Bf6f9Qv4AtQoJwtLw6+5pDXsTD5q/GwhPjt7ohF7aQZTMMHhZuS52/izKhDzIufl6uiqUBge 0lqG+/ViLKwCkxHDREuSUTtfjRc9/AoAt2V2HOfgKORSCjFC1eI0+8UMxlfdq2z1AAchinU0 eSkRpX2An3CPEjgGFmu2Je4a/R/Kd6nGU8AFaE8ta0oq5BSFDRYdcKchw4TSxetkG6iUtqOO ZFS7VAdF00eqFJNQpi6IUQryhnrOByw+zSobqlOPUO7XC5fjnwARAQABtCRHZW9yZ2UgVy4g RHVubGFwIDxkdW5sYXBnQHVtaWNoLmVkdT6JAlcEEwEKAEECGwMFCwkIBwMFFQoJCAsFFgID AQACHgECF4ACGQEWIQTXqBy2bTNXPzpOYFimNjwxBZC0bQUCXEowWQUJDCJ7dgAKCRCmNjwx BZC0beKvEACJ75YlJXd7TnNHgFyiCJkm/qPeoQ3sFGSDZuZh7SKcdt9+3V2bFEb0Mii1hQaz 3hRqZb8sYPHJrGP0ljK09k3wf8k3OuNxziLQBJyzvn7WNlE4wBEcy/Ejo9TVBdA4ph5D0YaZ nqdsPmxe/xlTFuSkgu4ep1v9dfVP1TQR0e+JIBa/Ss+cKC5intKm+8JxpOploAHuzaPu0L/X FapzsIXqgT9eIQeBEgO2hge6h9Jov3WeED/vh8kA7f8c6zQ/gs5E7VGALwsiLrhr0LZFcKcw kI3oCCrB/C/wyPZv789Ra8EXbeRSJmTjcnBwHRPjnjwQmetRDD1t+VyrkC6uujT5jmgOBzaj KCqZ8PcMAssOzdzQtKmjUQ2b3ICPs2X13xZ5M5/OVs1W3TG5gkvMh4YoHi4ilFnOk+v3/j7q 65FG6N0JLb94Ndi80HkIOQQ1XVGTyu6bUPaBg3rWK91Csp1682kD/dNVF3FKHrRLmSVtmEQR 5rK0+VGc/FmR6vd4haKGWIRuPxzg+pBR77avIZpU7C7+UXGuZ5CbHwIdY8LojJg2TuUdqaVj yxmEZLOA8rVHipCGrslRNthVbJrGN/pqtKjCClFZHIAYJQ9EGLHXLG9Pj76opfjHij3MpR3o pCGAh6KsCrfrsvjnpDwqSbngGyEVH030irSk4SwIqZ7FwLkBDQRUWmc6AQgAzpc8Ng5Opbrh iZrn69Xr3js28p+b4a+0BOvC48NfrNovZw4eFeKIzmI/t6EkJkSqBIxobWRpBkwGweENsqnd 0qigmsDw4N7J9Xx0h9ARDqiWxX4jr7u9xauI+CRJ1rBNO3VV30QdACwQ4LqhR/WA+IjdhyMH wj3EJGE61NdP/h0zfaLYAbvEg47/TPThFsm4m8Rd6bX7RkrrOgBbL/AOnYOMEivyfZZKX1vv iEemAvLfdk2lZt7Vm6X/fbKbV8tPUuZELzNedJvTTBS3/l1FVz9OUcLDeWhGEdlxqXH0sYWh E9+PXTAfz5JxKH+LMetwEM8DbuOoDIpmIGZKrZ+2fQARAQABiQNbBBgBCgAmAhsCFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKMJ4FCQnQ/OQBKcBdIAQZAQoABgUCVFpnOgAKCRCyFcen x4Qb7cXrCAC0qQeEWmLa9oEAPa+5U6wvG1t/mi22gZN6uzQXH1faIOoDehr7PPESE6tuR/vI CTTnaSrd4UDPNeqOqVF07YexWD1LDcQG6PnRqC5DIX1RGE3BaSaMl2pFJP8y+chews11yP8G DBbxaIsTcHZI1iVIC9XLhoeegWi84vYc8F4ziADVfowbmbvcVw11gE8tmALCwTeBeZVteXjh 0OELHwrc1/4j4yvENjIXRO+QLIgk43kB57Upr4tP2MEcs0odgPM+Q+oETOJ00xzLgkTnLPim C1FIW2bOZdTj+Uq6ezRS2LKsNmW+PRRvNyA5ojEbA/faxmAjMZtLdSSSeFK8y4SoCRCmNjwx BZC0bevWEACRu+GyQgrdGmorUptniIeO1jQlpTiP5WpVnk9Oe8SiLoXUhXXNj6EtzyLGpYmf kEAbki+S6WAKnzZd3shL58AuMyDxtFNNjNeKJOcl6FL7JPBIIgIp3wR401Ep+/s5pl3Nw8Ii 157f0T7o8CPb54w6S1WsMkU78WzTxIs/1lLblSMcvyz1Jq64g4OqiWI85JfkzPLlloVf1rzy ebIBLrrmjhCE2tL1RONpE/KRVb+Q+PIs5+YcZ+Q1e0vXWA7NhTWFbWx3+N6WW6gaGpbFbopo FkYRpj+2TA5cX5zW148/xU5/ATEb5vdUkFLUFVy5YNUSyeBHuaf6fGmBrDc47rQjAOt1rmyD 56MUBHpLUbvA6NkPezb7T6bQpupyzGRkMUmSwHiLyQNJQhVe+9NiJJvtEE3jol0JVJoQ9WVn FAzPNCgHQyvbsIF3gYkCYKI0w8EhEoH5FHYLoKS6Jg880IY5rXzoAEfPvLXegy6mhYl+mNVN QUBD4h9XtOvcdzR559lZuC0Ksy7Xqw3BMolmKsRO3gWKhXSna3zKl4UuheyZtubVWoNWP/bn vbyiYnLwuiKDfNAinEWERC8nPKlv3PkZw5d3t46F1Dx0TMf16NmP+azsRpnMZyzpY8BL2eur feSGAOB9qjZNyzbo5nEKHldKWCKE7Ye0EPEjECS1gjKDwQ==
  • Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 07 Feb 2019 17:21:47 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 2/5/19 1:38 PM, Roger Pau Monné wrote:
> On Tue, Feb 05, 2019 at 05:44:14AM -0700, Jan Beulich wrote:
>>>>> On 05.02.19 at 11:40, <roger.pau@xxxxxxxxxx> wrote:
>>> On Tue, Feb 05, 2019 at 12:45:56AM -0700, Jan Beulich wrote:
>>>>>>> On 04.02.19 at 18:18, <roger.pau@xxxxxxxxxx> wrote:
>>>>> On Mon, Feb 04, 2019 at 09:56:22AM -0700, Jan Beulich wrote:
>>>>>>>>> On 30.01.19 at 11:36, <roger.pau@xxxxxxxxxx> wrote:
>>>>>>> The assert was originally added to make sure that higher order
>>>>>>> regions (> PAGE_ORDER_4K) could not be used to bypass the
>>>>>>> mmio_ro_ranges check performed by p2m_type_to_flags.
>>>>>>>
>>>>>>> This however is already checked in set_mmio_p2m_entry, which makes
>>>>>>> sure that higher order mappings don't overlap with mmio_ro_ranges,
>>>>>>> thus allowing the creation of high order MMIO mappings safely.
>>>>>>
>>>>>> Well, the assertions were added to make sure no other code
>>>>>> path appears that violates this requirement. Arguably e.g.
>>>>>> set_identity_p2m_entry() could gain an order parameter and
>>>>>> then try to establish larger p2m_mmio_direct entries.
>>>>>>
>>>>>> Don't get me wrong, I don't object to the removal of the
>>>>>> assertions, but the description makes it sound as if they were
>>>>>> entirely redundant. Even better would be though if they
>>>>>> could be extended to keep triggering in "bad" cases.
>>>>>
>>>>> I could add something like:
>>>>>
>>>>> ASSERT(!rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
>>>>>                                 mfn_x(mfn) + PFN_DOWN(MB(2))));
>>>>>
>>>>> I think this should be safe and would trigger in case of misuse.
>>>>
>>>> Looks okay, if slightly extended (or made conditional) to exclude
>>>> the addition of MB(2) to MFN_INVALID to wrap and potentially
>>>> hit a r/o range in the low 1Mb.
>>>
>>> Ack, so it would be:
>>>
>>> ASSERT(mfn_eq(mfn, INVALID_MFN) ||
>>>        !rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
>>>                                 mfn_x(mfn) + PFN_DOWN(MB(2))));
>>
>> But that's still dropping the other aspect of the original ASSERT():
>>
>>>>>>> --- a/xen/arch/x86/mm/p2m-pt.c
>>>>>>> +++ b/xen/arch/x86/mm/p2m-pt.c
>>>>>>> @@ -668,7 +668,6 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t 
>>>>>>> gfn_, mfn_t mfn,
>>>>>>>          }
>>>>>>>  
>>>>>>>          ASSERT(p2m_flags_to_type(flags) != p2m_ioreq_server);
>>>>>>> -        ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
>>
>> It also made sure that "valid" MFNs can't be used for mappings with
>> p2m_mmio_direct type. Except that I realize now that this is wrong in
>> certain cases, because MMIO pages may actually have "valid" MFNs.
>> mfn_valid(), after all, only tells us whether there's a struct page_info
>> for the MFN. I wonder if it's really this brokenness that you hit,
>> rather than what is explained in the description.
>>
>> When the assertion was introduced, MMIO wasn't handled by the
>> code correctly anyway (!mfn_valid() MFNs would not have got any
>> mappings at all in the 2M and 1G paths), whereas now we have
>> p2m_allows_invalid_mfn() there. So the situation has become worse
>> with other nearby changes. As a result I think we want to correct
>> the assertion here alongside the addition of what you suggest
>> above. What about
>>
>>     if ( p2mt != p2m_mmio_direct )
>>         ASSERT(mfn_valid(mfn) || (mfn_eq(mfn, INVALID_MFN) &&
>>                p2m_allows_invalid_mfn(p2mt)));
>>     else
>>         ASSERT(!mfn_eq(mfn, INVALID_MFN) &&
>>                !rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
>>                                         mfn_x(mfn) + PFN_DOWN(MB(2))));

FWIW I agree with this approach (asserting !overlaps for p2m_mmio_direct
types).

 -George

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.